Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: single character wildcard matching #437

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jcosentino11
Copy link
Member

Issue #, if available:

Description of changes:

⚠️ WIP: still ironing out bugs

Adds new configuration option enableSingleCharacterWildcardMatching to allow ? matching in CDA policy resources. ? will match any single character, similar to how IoT Core handles it (https://docs.aws.amazon.com/iot/latest/developerguide/pub-sub-policy.html#pub-sub-policy-cert). Reason for flag is to ensure CDA version upgrade doesn't make policies already containing a ? more permissive than originally intended.

Why is this change necessary:

To maintain feature parity with IoT Core

How was this change tested:

Unit and integration tests

Any additional information or context required to review the change:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

return matches(s, true);
}

public boolean matches(String s, boolean matchSingleCharWildcard) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

The cyclomatic complexity of this method is 21. By comparison, 99% of the methods in the CodeGuru reference dataset have a lower cyclomatic complexity. This indicates the method has a high number of decisions and it can make the logic difficult to understand and test.

We recommend that you simplify this method or break it into multiple methods. For example, consider extracting the code block on lines 123-132 into a separate method.

@@ -48,23 +50,19 @@
* |
* </p>
*/
@Builder

Check notice

Code scanning / CodeQL

Use of default toString() Note

Default toString(): MetricsConfiguration inherits toString() from Object, and so is not suitable for printing.
Default toString(): SecurityConfiguration inherits toString() from Object, and so is not suitable for printing.
Default toString(): CAConfiguration inherits toString() from Object, and so is not suitable for printing.
Default toString(): RuntimeConfiguration inherits toString() from Object, and so is not suitable for printing.
Default toString(): DomainEvents inherits toString() from Object, and so is not suitable for printing.
}

@SuppressFBWarnings("EQ_DOESNT_OVERRIDE_EQUALS")
private static class DefaultHashMap<K, V> extends HashMap<K, V> {

Check failure

Code scanning / CodeQL

No clone method Error

No clone method, yet implements Cloneable.
Copy link

github-actions bot commented Sep 11, 2024

Benchmark Results

Benchmark Score
com.aws.greengrass.clientdevices.auth.benchmark.AuthorizationBenchmarks.GIVEN_policy_with_thing_name_variable_WHEN_auth_request_THEN_successful_auth 1265720.93 ops/s
com.aws.greengrass.clientdevices.auth.benchmark.AuthorizationBenchmarks.GIVEN_policy_with_wildcards_WHEN_auth_request_THEN_successful_auth 160693.88 ops/s
com.aws.greengrass.clientdevices.auth.benchmark.AuthorizationBenchmarks.GIVEN_single_group_permission_WHEN_simple_auth_request_THEN_successful_auth 2503156.18 ops/s

}
}

private class Node {

Check notice

Code scanning / CodeQL

Inner class could be static Note

Node should be made static, since the enclosing instance is not used.
return matches(root, s);
}

private boolean matches(@NonNull Node n, @NonNull String s) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

The cyclomatic complexity of this method is 17. By comparison, 99% of the methods in the CodeGuru reference dataset have a lower cyclomatic complexity. This indicates the method has a high number of decisions and it can make the logic difficult to understand and test. We recommend that you simplify this method or break it into multiple methods.

operations:
- "mqtt:subscribe"
resources:
- "mqtt:topic:${iot:Connection.Thing.ThingName}/???/test/*"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the combination of single level ? and multi level * valid in the same level and should we test for it? Ex: mqtt:topic:myThing/???* or mqtt:topic:myThing/*???

Copy link
Member Author

@jcosentino11 jcosentino11 Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Evaluation here is more regex-like than mqtt: * doesn't know anything about topic levels.

There should be test cases for this in WildcardTrieTest already (not all are passing quite yet)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants